Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ellipsoid geometry #1221

Merged
merged 7 commits into from
Mar 11, 2025
Merged

Add ellipsoid geometry #1221

merged 7 commits into from
Mar 11, 2025

Conversation

aprokop
Copy link
Contributor

@aprokop aprokop commented Mar 8, 2025

  • Add Exprimental::Ellipsoid
  • Provide intersections with point, segment and box (2D only)

@aprokop aprokop added the enhancement New feature or request label Mar 8, 2025
@aprokop aprokop merged commit 64b47fc into arborx:master Mar 11, 2025
1 of 2 checks passed
@aprokop aprokop deleted the ellipsoid branch March 11, 2025 20:39
Copy link
Contributor

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Posting in progress review

KOKKOS_FUNCTION
constexpr Ellipsoid(
Point<DIM, Coordinate> const &centroid,
std::initializer_list<std::initializer_list<Coordinate>> const rmt)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we want this constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For unit tests to allow {{a, b}, {b, c}} initialization.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could(should?) have used array of arrays

constexpr auto const &centroid() const { return _centroid; }

KOKKOS_FUNCTION
constexpr auto const &rmt() const { return _rmt; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's RMT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Riemann metric tensor

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment...

#else
KOKKOS_FUNCTION
#endif
Ellipsoid(T const (&)[N], T const (&)[N][N]) -> Ellipsoid<N, T>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we have this deduction guide and a constructor from an initializer lists instead of having a constructor from array of arrays?

Copy link
Contributor Author

@aprokop aprokop Mar 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was trying to make things work well enough. If you have a better approach that would simplify things, it would be nice. I want to be able to construct ellipsoid as

Ellipsoid ellipse{{3, 1}, {1, 3}}; // testing
float rmt[2][2];
// initialize rmt
Ellipsoid ellipse1{Point{0,0}, rmt};

@@ -512,6 +515,123 @@ struct intersects<BoxTag, SegmentTag, Box, Segment>
}
};

namespace
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why anonymous space here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I try to use anonymous spaces when I know it's not needed anywhere outside of this file.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a header file though

{
// Computes x^t R y
template <int DIM, typename Coordinate>
KOKKOS_INLINE_FUNCTION auto rmt_multiply(Details::Vector<DIM, Coordinate> x,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any good reason not to spell out that it returns Coordinate instead of auto?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No good reason. I find either acceptable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case it is slightly less readable

Comment on lines +545 to +553
template <typename Point, typename Ellipsoid>
struct intersects<PointTag, EllipsoidTag, Point, Ellipsoid>
{
KOKKOS_FUNCTION static constexpr bool apply(Point const &point,
Ellipsoid const &ellipsoid)
{
return Details::intersects(ellipsoid, point);
}
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we generate intersects(geom2, geom1) from instersects(geom1, geom2)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't. If you know of an easy way to do it, I'm all ears. Boost geometry does it, but I can't quite recall why we couldn't do it that way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants